-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add fake clock for test code #5304
Conversation
@@ -1826,10 +1856,10 @@ class MemoryQueueTests | |||
) | |||
|
|||
records.foreach(record => queue = queue.enqueue(record)) | |||
|
|||
Thread.sleep(5000) | |||
clock.plusSeconds(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- - Thread.sleep(5000)
+ + clock.plusSeconds(5)
Change the thread sleep-based test to use fake clock.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5304 +/- ##
==========================================
- Coverage 81.24% 76.10% -5.14%
==========================================
Files 238 239 +1
Lines 14178 14192 +14
Branches 579 580 +1
==========================================
- Hits 11519 10801 -718
- Misses 2659 3391 +732 ☔ View full report in Codecov by Sentry. |
@@ -151,11 +164,11 @@ class MemoryQueueFlowTests | |||
container.send(fsm, getActivation(false)) | |||
container.expectMsg(ActivationResponse(Left(NoActivationMessage()))) | |||
|
|||
Thread.sleep(idleGrace.toMillis) | |||
fsm ! StateTimeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can this make sure the timeout happens after idleGrace
and stopGrace
respectively?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When idleGrace
and stopGrace
times out, a StateTimeout message is sent.
It explicitly sends a StateTimeout message, which is exactly the same as a message sent by timeout .
when(Running, stateTimeout = queueConfig.idleGrace) {
when(Idle, stateTimeout = queueConfig.stopGrace) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but I feel the semantic is different.
For example, if anyone removes the stateTimeout
configuration in each state in MemoryQueue by mistake, the previous code can guarantee that it should be added.
But the changed code cannot guarantee it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@style95 I think what you said can be guaranteed with separate test code waiting for timeout.
For example, with a very short time timeout setting, the test code can wait as much as the actual timeout. I'll add some test code that covers what you said. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, that would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@style95 Since I verified that the state timeout works normally in each state, I think that the test code in MemoryQueueFlowTests is already covered at the same level as before (StateTimeout is explicitly called). Is there anything I've missed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant what you added here: c1de659#diff-042a5fb7a94e1f4755cd38f9d3892ceed3db93e54cb7d34bd9bdf8289ef7c7fcR185
covers the timeout for the normal flow.
But there are many cases for other timeouts such as flushGrace
and gracefulShutdownTimeout
.
The MemoryQueueFlowTests guarantees the behavior of the memory queue in order, so I just want to make sure its coverage is not changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@style95
I've already covered idleGrace
, stopGrace
, gracefulShutdownTimeout
you said. I'll add test for flushGrace
as well. flushGrace
seems to have been added recently.
- idleGrace
- stopGrace
- gracefulShutdownTimeout
According to the state timeout config for each FSM state, the actual time passes and the state is changed when timeout. I ensured that timeout works well in each state with the test code based on timer (thread sleep).
Because I tested sending FSM StateTimeout when real time passes, explicitly sending StateTimeout ensures the same level of test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@style95 A new test code is added for onTransition(Transition _ => Flushing). It checks if the timer StopQueue sending StateTimeout is active after the state transitions to Flushing.
// Test case _ -> Flushing => startTimerWithFixedDelay("StopQueue", StateTimeout, queueConfig.flushGrace)
// state Running -> Flushing
expectMsg(Transition(fsm, Running, Flushing))
fsm.isTimerActive("StopQueue") shouldBe true
And also, it checks whether StateTimeout msg is actually received by waiting for flush grace time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add fake clock for test code * Add test code for state timeout * Add test case for transaction _ => Flushing * Add StateTimeout test for Flushing state
Description
There is an issue that the test results are different depending on the performance of the build machine because the test code is written in a time-dependent. This PR improves memory queue test code that fails like heisenbug.
Related issue and scope
My changes affect the following components
Types of changes
Checklist: